Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the rust hyper client Send so it can be used in rust threads more easily #19207

Closed
wants to merge 9 commits into from

Conversation

blazzy
Copy link
Contributor

@blazzy blazzy commented Jul 20, 2024

The hyper client cannot be used easily in threads as it isn't Send friendly. Something like this will fail to compile:

use petstore_hyper::apis::client::APIClient;
use petstore_hyper::apis::configuration::Configuration;

#[tokio::main]
async fn main() {
    tokio::spawn(async move {
        let client = APIClient::new(Configuration::new());
        let _result = client.pet_api().get_pet_by_id(5).await;
    });
}

Issue with more detailed steps to reproduce: #19208

This PR makes the hyper client and its internals Send. So the client can be used in threads.

It ended up being a little larger of a PR in the pursuit of writing a test. It turns out the tests aren't actually being run when invoking verify on the petstore samples. This PR had to touch a few other thing to ensure cargo test would work. Let me know if I should break it up into multiple PRs.

Should I update the pom.xml file to do a cargo test instead of a cargo check? I'm not sure if that was intentional.

Commit summary

This might be a good order to review the changes:

7ee4bca - This is the commit that actually makes the code work in threads. I adds the Send constraint to a bunch generic parameters and traits and uses an Arc instead of an Rc

bda9b6d - This commit adds a test to validate that it works and doesn't regress.

e53c04c - This commit regenerates the samples

The remaining commits were trying to get the tests to run in samples/client/others/rust/ to run successfully. There were several issues to fix to get tests to run successfully.

43ba4f0 46d3520 These set it up so the doc strings are compilable. The first one adds a property called externCrateName cribbing from the rust-server generator. The second one uses the externCrateName and fixes other issues with the example rustdoc examples.

ddb330c The reqwest tests were not compiling and needed a small change.

efbf445 This changes the petstore swagger file and generates the petstore tests to use https. http://petstore.swagger.io/v2 is 301 redirecting to https://petstore.swagger.io/v2 and this is breaking POSTS.
When the client recieves a redirect it does not resend the POST data, instead it switches to GET. This is in line with how browsers behave when encountering a 301 redirect on a POST request, so I wouldn't consider it a bug in the client, but instead a bug in the test code.

Pinging the rust technical committee members: @frol @farcaller @richardwhiuk @paladinzh @jacob-pro

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh ./bin/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    (For Windows users, please run the script in Git BASH)
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.6.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

blazzy added 6 commits July 17, 2024 15:12
This is follows the lead of the rust hyper server generator and provides
an externCrateName. This is because the crate name used for importing
can be different from the package name, because dashes `-` get converted
to underscores `_`.

This allows us to write example code in rustdoc that compiles
successfully.
http://petstore.swagger.io/v2 is 301 redirecting to
https://petstore.swagger.io/v2 and this is breaking posts to the API.
When the client recieves a redirect it does not resend the POST data,
instead it switches to GET. This is in line with how browsers behave
when encountering a 301 redirect on a POST request.
@blazzy blazzy force-pushed the hyper-client-send branch from efbf445 to 31c6a3b Compare July 20, 2024 01:54
@blazzy blazzy changed the title Make the rust hyper client Send so it can be used in rust threads Make the rust hyper client Send so it can be used in rust threads more easily Jul 22, 2024
@Setter protected String packageVersion = "1.0.0";
protected String apiDocPath = "docs/";
protected String modelDocPath = "docs/";
protected String apiFolder = "src/apis";
protected String modelFolder = "src/models";
private String externCrateName = "openapi";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of adding another variable, what about simply create an function called getCrateName() and we can keep the @Setter above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Why add more state. I was just blindly copying from the rust server code gen.

Updated in the latest commit: 5f8bda3

blazzy added 2 commits July 23, 2024 07:01
This trait is also helpful in making the api work well with threads.
@blazzy blazzy force-pushed the hyper-client-send branch from 93a6df5 to 5f8bda3 Compare July 23, 2024 14:14
@blazzy
Copy link
Contributor Author

blazzy commented Jul 23, 2024

Added one more small commit fa413b4 that makes the api structs Sync too. Helpful for using the struct with threads.

@wing328
Copy link
Member

wing328 commented Jul 24, 2024

thanks for the PR. Is it correct to say that this PR is a breaking change? (we do allow breaking changes but need to roll out carefully)

@blazzy
Copy link
Contributor Author

blazzy commented Aug 1, 2024

I'd imagine if the user is just importing APIClient and Configuration and just using those they would be fine.

But if they were importing the different traits and implementing the traits for some reason they would run into compatibility issues. Also if they were importing the individual API structs as opposed instantiating them through the APIClient functions they would need to pass an Arc instead of an Rc.

So it's technically not backwards compatible. Though maybe backwards compatible for the most common expected use cases?

I'm not actually sure why the traits exist to be honest. It seems to add a little more complexity for a pay off that is mysterious to me at the moment.

@blazzy
Copy link
Contributor Author

blazzy commented Aug 1, 2024

I'm not actually sure why the traits exist to be honest. It seems to add a little more complexity for a pay off that is mysterious to me at the moment.

Oh. I guess it's to mask the generic parameters for the individual api structs by wrapping them in Box<dyn trait>. That makes the APIClient a little more pleasant to use.

@wing328 wing328 added Client: Rust Enhancement: Code Cleanup General refactoring, removal of deprecated things, commenting, etc. labels Aug 4, 2024
@wing328 wing328 added this to the 7.8.0 milestone Aug 4, 2024
@wing328
Copy link
Member

wing328 commented Aug 15, 2024

can you please resolve the merge conflicts when you've time? i'll try to get it merged for the upcoming release

@@ -1,6 +1,6 @@
openapi: 3.0.0
servers:
- url: 'http://petstore.swagger.io/v2'
- url: 'https://petstore.swagger.io/v2'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wing328
Copy link
Member

wing328 commented Aug 17, 2024

merged via #19375

(tested locally with local petstore server)

thanks again for the PR

@wing328 wing328 closed this Aug 17, 2024
@blazzy
Copy link
Contributor Author

blazzy commented Aug 23, 2024

Thanks! Sorry I didn't have time to swing back to it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client: Rust Enhancement: Code Cleanup General refactoring, removal of deprecated things, commenting, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants